fix: avoid loading UserRole members during JSON Patch apply [DHIS2-21852] - #24489
Merged
Conversation
jbee
reviewed
Jul 20, 2026
jbee
left a comment
jbee
left a comment
Contributor
There was a problem hiding this comment.
I don't like how the mechanics of the filtering work. We have a constant hard coded into a jackson mapping filter as well as into a @JsonFilter annotation on a mixin thing. Way to complicated and entangled to be flexible or extendable for other cases. I think we should keep this simple and just hard code this as explicit and hard-coded as we can make it ideally just being in one place.
As I understand this approach it also means you cannot patch the excluded property even if targeted explicitly. If that is the case maybe we should throw an exception if the user request does attempt such a patch?
Contributor
|
netroms
marked this pull request as draft
July 21, 2026 07:52
Contributor
Author
Follow-up on system-wide PATCH side effectsThanks — valid concern given this sits in Extra coverage added
Skip rules (unchanged for collections; non-persisted props)
PUT |
6 tasks
netroms
added a commit
that referenced
this pull request
Jul 23, 2026
Spec for multi-entity integration + web-api coverage answering David's system-wide PATCH concern on PR #24489 / DHIS2-21852.
netroms
added a commit
that referenced
this pull request
Jul 23, 2026
Task breakdown for multi-entity integration + web-api coverage on PR #24489 / DHIS2-21852.
netroms
force-pushed
the
fix-userrole-jsonpatch-lazy-members
branch
from
July 23, 2026 14:24
b55c194 to
f12cfba
Compare
jason-p-pickering
added a commit
that referenced
this pull request
Jul 23, 2026
…FilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap<Class<?>, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jason-p-pickering
added a commit
that referenced
this pull request
Jul 23, 2026
…FilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap<Class<?>, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jason-p-pickering
added a commit
that referenced
this pull request
Jul 23, 2026
…FilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap<Class<?>, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
netroms
marked this pull request as ready for review
July 24, 2026 10:53
jason-p-pickering
added a commit
that referenced
this pull request
Jul 26, 2026
…FilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap<Class<?>, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jason-p-pickering
force-pushed
the
fix-userrole-jsonpatch-lazy-members
branch
from
July 26, 2026 06:48
03adef3 to
8b977fc
Compare
jason-p-pickering
added a commit
that referenced
this pull request
Jul 26, 2026
Calibrated from the baseline/candidate A/B run on the platform-perf DB used to recalibrate PR #24489 after the rebase onto the JsonPatchFilterMixin refactor. PATCH scenarios share one threshold since the invariant under test is that PATCH latency must not depend on role membership size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jbee
approved these changes
Jul 27, 2026
jbee
left a comment
jbee
left a comment
Contributor
There was a problem hiding this comment.
I think the filter and filter mixin could be package locale or even inner classes of the manager as their function is only internal and unlikely to be reused in other contexts.
JsonPatchManager converted the managed entity to a tree via valueToTree and then re-invoked every collection getter in handleCollectionUpdates. For UserRole, getUsers() initializes all lazy members, causing O(members) SQL and hundreds of MB of allocation on scalar PATCHes. Skip non-owner collection properties that no patch path references, in both serialization passes. Non-owner collections are ignored by metadata import UPDATE, so omitting them is semantically free; owner collections always stay in the tree (omitting them would clear them on import). Non-owner collections referenced by patch paths keep today's behavior.
Gatling simulation patching a scalar field on an empty control role and on a large-membership role (platform-perf DB), making the O(members) PATCH regression visible. No calibrated thresholds yet; asserts 100% success only.
OrganisationUnit.leaf calls children.isEmpty(), which initializes the inverse children collection even when JsonPatchManager already omits non-owner collections. Exclude unreferenced non-persisted properties from patch serialization so derived getters cannot force lazy loads.
…FilterService pattern [DHIS2-21852] Addresses review feedback on #24489 (Jan Bernitt): the per-call `.addMixIn(realClass, ...)` + inline mixin + filter-provider wiring in JsonPatchManager.apply() was rebuilt from scratch on every call and didn't follow any existing convention. Reworks it to mirror the codebase's established pattern for the same underlying problem -- skipping a getter during Jackson serialization based on per-call criteria, without invoking it -- already used for the `?fields=` GET path (org.hisp.dhis.fieldfiltering.FieldFilterService/FieldFilterMixin). - New `JsonPatchFilterMixin` (`@JsonFilter`) and `JsonPatchExcludedPropertyFilter` (`SimpleBeanPropertyFilter`), shaped like `FieldFilterMixin`/`FieldFilterSimpleBeanPropertyFilter`. - `JsonPatchManager` caches one mixin-bound `ObjectMapper` copy per entity type (`ConcurrentHashMap<Class<?>, ObjectMapper>`, built lazily via `computeIfAbsent`) instead of rebuilding the mixin binding on every `apply()` call. `findExcludableNonOwnerCollections` (the exclusion rule itself) is unchanged -- it was already schema-driven and generic. - The mixin is scoped per-`realClass`, not bound to `Object.class` globally: `FieldFilterMixin`'s own filter is path-aware, so a global binding is safe for it, but `JsonPatchExcludedPropertyFilter` matches by property name only. A global binding was tried first and found, in review, to silently strip `Sharing.users`/`Sharing.userGroups` (present on every `IdentifiableObject`) whenever a same-named top-level collection was excluded, e.g. on a scalar UserRole PATCH. Scoping per-class makes that cross-type collision impossible. Regression test: `testUserRoleSharingUsersSurviveScalarPatch`. - Answers Jan's second question (should an explicit patch to an excluded property throw?): no -- `findExcludableNonOwnerCollections` already un-excludes a property referenced by a patch path, so it falls back to full (slower, correct) hydration rather than being silently dropped. Replaced the previous `testUserRoleUsersPathPatchDoesNotThrow` (asserted only `assertNotNull`, would pass either way) with `testUserRoleUsersPathPatchFallsBackToFullHydration`, which asserts `Hibernate.isInitialized(...)` actually flips to `true`, mutation-tested against the fallback logic. No functional change to PATCH behavior beyond fixing the Object.class regression introduced and caught within this same review cycle (never released). All existing JsonPatchManagerTest coverage (14 tests) plus the new JsonPatchExcludedPropertyFilterTest (2 tests) pass; UserControllerTest (52 tests, dhis-test-web-api) unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Calibrated from the baseline/candidate A/B run on the platform-perf DB used to recalibrate PR #24489 after the rebase onto the JsonPatchFilterMixin refactor. PATCH scenarios share one threshold since the invariant under test is that PATCH latency must not depend on role membership size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
netroms
force-pushed
the
fix-userrole-jsonpatch-lazy-members
branch
from
July 27, 2026 17:46
a77739b to
fbe6983
Compare
Per review: both are internal to JsonPatchManager and not reused outside the package, so drop public visibility.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #24489 +/- ##
=============================================
+ Coverage 35.47% 69.53% +34.06%
- Complexity 533 726 +193
=============================================
Files 3713 3714 +1
Lines 143929 144340 +411
Branches 16776 16839 +63
=============================================
+ Hits 51064 100374 +49310
+ Misses 88660 36234 -52426
- Partials 4205 7732 +3527
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2166 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
david-mackessy
approved these changes
Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PATCH /api/userRoles/{uid}(JSON Patch) hydrated the entire lazyUserRole.memberscollection even for scalar-only patches, making a one-column update cost O(members) time and allocation. Production trace: ~4s wall / ~707 MB alloc for a role with ~320 members, where the useful work was oneUPDATE userrole.Root cause
JsonPatchManager.applyserializes the live Hibernate entity twice:valueToTreeinvokes every@JsonPropertygetter (includingUserRole.getUsers(), which iterates the lazymembersset), andhandleCollectionUpdatesthen re-invokes every schema collection getter. The serializedusersdata is thrown away:UserRolehas nosetUsers, and the metadata importer ignores non-owner properties on UPDATE.Fix
Skip collection properties that are non-owner and not referenced by any patch path, in both serialization passes:
valueToTreeuses a per-call mapper copy with a@JsonFiltermixin bound to the entity class only (excluded getters are never invoked; nested objects serialize unchanged; fast path when nothing is excluded).handleCollectionUpdatesskips excluded properties beforesafeInvoke.Owner collections (e.g.
authorities) always stay serialized, since omitting them would clear them on import UPDATE. Non-owner collections referenced by patch paths behave exactly as before. This also covers other fat inverse collections (e.g.UserGroupmembers) without per-entity special cases.Performance
Ran via the
performance-tests-compareworkflow on the self-hosted performance runner: platform-perf DB (~250k users), baselinedhis2/core-dev:latestvs candidatedhis2/core-pr:24489(this PR's rebased HEAD,8b977fcdb2a, includes theJsonPatchFilterMixinrework from review),PATCH replace /description, Gatling sequential, 10 iterations:Candidate latency is independent of membership size (~99.8% faster / ~583x at 83k members). GET (control) unaffected. SQL statement log confirms the
userrolemembershydrate query is gone and membership rows are unchanged after PATCH.Testing
JsonPatchManagerTest: new invariant test assertingHibernate.isInitialized(role.getMembers())stays false after a scalar patch apply; owner-collection (authorities) preservation;/users-path parity. 13/13 green.UserControllerTest: PATCH description on a role with users returns 200, description updated, users retained on GET.UserRolesPerformanceTestGatling simulation (used for the numbers above; no calibrated thresholds yet, asserts 100% success).JIRA: DHIS2-21852
AI Assisted